Skip to content

split MCP OAuth teardown ownership from MCP 2.0 transport composition - #84963

Open
andrexibiza wants to merge 23 commits into
NousResearch:mainfrom
andrexibiza:agent/close-mcp-oauth-lock-class
Open

split MCP OAuth teardown ownership from MCP 2.0 transport composition#84963
andrexibiza wants to merge 23 commits into
NousResearch:mainfrom
andrexibiza:agent/close-mcp-oauth-lock-class

Conversation

@andrexibiza

@andrexibiza andrexibiza commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Current architecture disposition

Do not merge or monolithically rebase this 23-commit PR. Split it into two credited children, then close this PR as superseded by the credited split once both child PRs exist.

  1. OAuth lifecycle / callback ownership childtools/mcp_oauth.py, tools/mcp_oauth_manager.py, and their focused tests. Preserve deterministic delegated-generator close on the owner task, provider generation fencing, failed-close retry, atomic first-writer-wins callback arbitration, lock-consistent waiter snapshots, durable token cleanup after failed refresh, and the real teardown/callback witnesses.
  2. MCP 2.0 transport/control-plane composition child — restack only the surviving mcp_tool.py and HTTPX/OAuth wire integration on the current negotiation architecture after WIP(mcp): dual-era hardening — bounded -32602 proof probe pending #88875. Prove legacy Streamable HTTP, current stateless Streamable HTTP, SSE where supported, and both Hermes-served MCP surfaces. Do not introduce a second era selector, transport factory, or cache identity.
  3. Discovery separation — MCP server/discover/initialize selects protocol era; PRM/ASM/CIMD select OAuth control-plane metadata. Failure in one axis must not silently select the other. CIMD feeds the same validated OAuth metadata model; it is not a bypass or second provider cache.
  4. Provenance — preserve @THArrowofApollo’s canonical Co-authored-by credit and the substantial teardown/callback evidence in the lifecycle child and in any transport child that carries code derived from this branch.

This PR therefore Refs #38193. It must not auto-close the issue before the lifecycle child lands and is verified on current main.

Related: #49543, #81051, #88875.

What this PR proved

This branch built a credible closure candidate for the MCP OAuth teardown-lock failure class across the then-current public HTTP transport surfaces:

  • deterministically closes the delegated SDK auth-flow generator from the owning task;
  • materializes OAuth resource bearer state without leaving the SDK auth provider attached to long-lived data-plane streams;
  • composes the OAuth HTTPX client factory for then-current Streamable HTTP, legacy Streamable HTTP, and default/custom SSE paths;
  • applies final-wire bearer isolation after user request hooks;
  • preserves legitimate OAuth token/refresh client authorization using SDK-captured request provenance, independent of token-endpoint pathname;
  • strips authorization on cross-origin redirects and carries irreversible chain taint, so an A → B → A redirect cannot restore credentials;
  • arbitrates browser callbacks, pasted callback values, and user skip as atomic first-writer-wins outcomes;
  • reads callback code, state, and error through one lock-consistent snapshot, preventing timeout or a concurrent callback from observing a torn result.

Those lifecycle and callback invariants remain valuable. The transport integration is no longer source-of-truth because current main and #88875 have since changed protocol negotiation, MRTR, listen supervision, stateless liveness, cache identity, and real dual-SDK wire coverage.

Why

The SDK OAuth flow holds an AnyIO lock across async-generator yields. If Hermes closes only its outer bridge after transport teardown, the inner generator can be finalized from another task, fail AnyIO's ownership check, and leave the cached provider permanently locked. Long-lived HTTP streams also require explicit separation between OAuth control traffic and resource data-plane bearer headers.

The published predecessor exposed one additional loaded-CI race: callback timeout polling could beat an already-started non-interactive skip reader. Adversarial review then found that HTTP and paste writers—and later the waiter read—needed one shared arbitration boundary. The final object atomically publishes and snapshots terminal callback state while preserving deterministic thread and listener cleanup.

Attribution

This is the credited successor to the original patch by @THArrowofApollo. The publication range contains exactly one canonical Co-authored-by trailer preserving that authorship and no newly introduced Signed-off-by trailer. Any child extraction must preserve that credit rather than treating this PR as obsolete or duplicate.

Historical verification evidence

Exact historical candidate: 9721dab3fb2b2dc0bb000abbb55498b85db72b09
Tree: 3ebdff9c6e679d358fa84077436b662203c0220c

Synthetic verification against then-live main 8018f9e016a934fe68174ddfa6e7e735956a4c54:

  • 274 passed, 1 skipped, 0 failed across 13 files;
  • 10/10 real OAuth witnesses passed;
  • synthetic merge tree: e35e2a8179177748997d5e06bcbb04511d91efd3;
  • receipt SHA-256: 8b36ee007d1b204b200318776167dac425d2e9450b48c69146b89e8726b6bce0;
  • candidate tree closure: 8,752/8,752 entries present;
  • Ruff, py_compile, attribution audit, and git diff --check passed.

Fresh exact-candidate verification also passed:

  • canonical sequential callback/OAuth surface: 120 passed, 1 skipped;
  • synchronized partial-publication stress: 50/50 passed;
  • independent candidate-tree runs: 39 remediation tests; 43 base OAuth tests plus 1 skip; 38 real witnesses;
  • concurrent arbitration probes: 1,100 first-writer rounds and an independent 100-thread contention probe.

Two independent exact-object reviewers approved the same historical SHA/tree after verifying shared-lock coverage for every callback result writer and waiter decision, candidate-tree closure, publication-range attribution, transport composition, OAuth provenance, and irreversible A → B → A redirect taint.

The later POSIX skip-token regression was repaired at current PR head 6ab18eb3c7f26b3c01df4b68b01b2bf3d3b4bb38 and its focused OAuth suites passed. None of this old-head evidence substitutes for current-main verification of either child.

Type of change represented

  • Bug fix
  • Security hardening
  • Tests

Historical files

  • tools/mcp_oauth.py
  • tools/mcp_oauth_manager.py
  • tools/mcp_tool.py
  • focused MCP OAuth and transport regression tests

igorhvr and others added 18 commits August 11, 2026 11:46
An OAuth MCP server could park with a bare `TimeoutError` and stay
unusable for the life of the process, while its endpoint and tokens were
both healthy.

`get_or_build_provider()` caches one `OAuthClientProvider` per server and
only discards it when `server_url` changes. That provider holds two
asyncio primitives bound to the loop that first awaited them: the entry's
own `lock`, and the SDK's `context.lock`, which
`mcp/client/auth/oauth2.py` holds across the whole auth flow (including
its `yield`s).

MCP runs on a dedicated loop created by `_ensure_mcp_loop()`.
`shutdown_mcp_servers()` stops that loop and clears `_servers` on every
MCP reload, but never touches the OAuth cache — and its inner shutdown is
bounded by `future.result(timeout=15)`, whose failure is only logged at
debug before `_stop_mcp_loop()` runs regardless. An auth flow still in
flight at that moment is stranded holding the lock, on a loop that will
never run again.

The next connect reuses that provider, so `session.initialize()` blocks on
a lock no live loop can release. It never reaches the wire: the only
symptom is the empty `TimeoutError` from the `asyncio.wait_for` in the
streamable-HTTP connect path, which reads as a network fault and sends you
looking at egress or credentials. Nothing recovers it in-process — the
parked self-probe rebuilds the transport but reuses the same poisoned
provider — so the server stays down until restart. Observed in the wild as
two multi-hour outages across separate PIDs, ~19h and ~18h, against a
reachable endpoint with valid tokens.

Record the binding loop on `_ProviderEntry` and treat a stopped or closed
one like a URL change: drop the entry and rebuild. Tokens live on disk, so
a rebuild reloads them and cannot trigger re-auth — `_build_provider`
only raises `OAuthNonInteractiveError` when no cached tokens exist.

Deliberately narrow: only a stopped/closed loop invalidates. A
different-but-live loop is fine, since an uncontended lock rebinds on next
acquire, and an unrecorded loop (sync CLI paths) fails open — churning
providers would discard live OAuth state for no reason.

Affects OAuth HTTP servers only; stdio servers have no provider and were
never impacted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Close delegated SDK auth generators deterministically across completion, failure, cancellation, and outer close; fence provider reuse across construction-config and event-loop ownership changes. Preserve response-driven OAuth semantics and callback/redirect configuration.

Co-authored-by: THArrowofApollo <144398232+THArrowofApollo@users.noreply.github.com>
Co-authored-by: THArrowofApollo <tharrowofapollo@users.noreply.github.com>
Co-authored-by: THArrowofApollo <tharrowofapollo@users.noreply.github.com>
Co-authored-by: THArrowofApollo <tharrowofapollo@users.noreply.github.com>
Co-authored-by: THArrowofApollo <tharrowofapollo@users.noreply.github.com>
Co-authored-by: THArrowofApollo <tharrowofapollo@users.noreply.github.com>
Co-authored-by: THArrowofApollo <tharrowofapollo@users.noreply.github.com>
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tools Tool registry, model_tools, toolsets tool/mcp MCP client and OAuth area/auth Authentication, OAuth, credential pools sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 13, 2026
The stop-aware rework of _paste_callback_reader made the POSIX branch
poll stdin with select.select, which raises TypeError for a wrapper
without a native descriptor (e.g. a unittest.mock stdin in tests).
The reader then returned without reading, so a completed `skip`
never landed and _wait_for_callback misreported it as a timeout
instead of raising OAuthNonInteractiveError("user_skipped") — the
exact "skip misreported as timeout" class this series set out to
close.

Mirror the Windows branch's mock handling: when stdin has no native
fd, fall back to a direct readline for mocked/in-memory streams
while keeping the select-polling loop for real descriptors.

Signed-off-by: Axl Ibiza, MBA <andrexibiza@gmail.com>
@andrexibiza

Copy link
Copy Markdown
Contributor Author

CI repair — slice 3/12 test_skip_raises_non_interactive_error

Verdict: genuine regression in this PR's surface (not pre-existing). The failing test file tests/tools/test_mcp_oauth.py is byte-identical to origin/main (blob 99e724f2), but tools/mcp_oauth.py is touched by this PR, and the PR's stop-aware rework of _paste_callback_reader broke the skip-token path on POSIX.

Root cause: the POSIX branch polls stdin with select.select([sys.stdin]). With a descriptor-less wrapper (e.g. the test's MagicMock stdin), select raises TypeError: fileno() returned a non-integer → caught by the broad except → the reader returns without reading → the completed skip never lands → _wait_for_callback misreports it as a timeout instead of raising OAuthNonInteractiveError("user_skipped"). Windows passed locally only because its branch retains explicit MagicMock handling; Linux CI failed.

Proof:

  • Merge-state run (branch merged with current origin/main d16e236): failure reproduced, exact CI message (Expected regex: 'user_skipped', actual timeout message).
  • File-swap proof: git checkout origin/main -- tools/mcp_oauth.py → test passes on pristine main module; restore → fails again.
  • Local POSIX-branch simulation: before fix, skip landed: None; after fix, OAuthNonInteractiveError = user_skipped.

Fix (commit 6ab18eb3): mirror the Windows branch's mock handling in the POSIX branch — when stdin has no native fileno() descriptor, fall back to a direct readline() for mocked/in-memory streams; keep the select.select polling loop for real descriptors.

Test evidence (main venv, --basetemp=C:/HERMES-TEMP/ci-84963):

  • tests/tools/test_mcp_oauth.py — 49 passed, 1 skipped
  • PR's own surface (test_mcp_oauth_bidirectional/lock_lifecycle/manager/metadata/real_witnesses/review_remediation.py) — 103 passed
  • Combined rerun on final commit — 55 passed, 1 skipped
  • ruff check tools/mcp_oauth.py — clean (format churn at lines 1442-1472 is pre-existing at PR head, unrelated; not touched per no-churn gate)
  • scripts/audit_pr_attribution.py — ✅ all contributor emails mapped

Branch pushed fast-forward to fork agent/close-mcp-oauth-lock-class: 9721dab3fb2..6ab18eb3c7f. Merge-tree vs current main: clean (exit 0).

Copy link
Copy Markdown
Contributor Author

Current architecture decision after reconciling this draft with MCP 2.0 and #88875:

Do not rebase all 23 commits monolithically. Split the surviving ownership fix from the transport-era adaptation, preserve @THArrowofApollo provenance on both, and restack the transport half on the current MCP 2.0 negotiation surface.

Why:

  • The branch is now 1,688 commits behind current main and tools/mcp_tool.py is a 1,156-line diff against a surface that WIP(mcp): dual-era hardening — bounded -32602 proof probe pending #88875 has since substantially rewritten for legacy/stateless negotiation, MRTR, listen supervision, liveness, cache identity, and real dual-SDK wires.
  • The core teardown/callback correctness work is still absent from current main: current mcp_oauth_manager.py does not contain the delegated-flow ownership registry (_hermes_active_flows), generation fence, owner-loop cleanup/retry path, or the branch's atomic callback arbitration. That work remains real and should not be discarded.
  • The branch also carries transport/control-plane behavior that now has to be re-proven against MCP 2.0 semantics rather than replayed from the 1.28-era integration shape: OAuth request classification, metadata discovery, HTTPX client composition, redirect taint, and mcp_tool.py transport wiring.

Recommended split:

  1. OAuth lifecycle / callback ownership PRtools/mcp_oauth.py, tools/mcp_oauth_manager.py, and focused tests. Keep deterministic delegated-generator close on the owner task, provider generation fencing, failed-close retry, callback first-writer-wins state, lock-consistent waiter snapshots, durable token cleanup after failed refresh, and the exact real-witness tests that prove those invariants. This is the direct credited successor to @THArrowofApollo's teardown finding.

  2. MCP 2.0 transport/control-plane composition PR — restack only the surviving mcp_tool.py and HTTPX/OAuth wire integration onto current main after WIP(mcp): dual-era hardening — bounded -32602 proof probe pending #88875's negotiation model is treated as source truth. Prove all served HTTP surfaces: legacy Streamable HTTP, current stateless Streamable HTTP, SSE where still supported, and the Hermes tools server/client surfaces. Do not reintroduce a second protocol-era selector or transport factory.

  3. Discovery/CIMD decision — treat stateless negotiation and OAuth metadata discovery as separate axes. server/discover/initialize decides MCP protocol era; PRM/ASM/CIMD decide OAuth client/control-plane metadata. A failure in one must not silently select the other. CIMD support should feed the same validated OAuth control-plane model, not become a second provider cache identity or a reason to bypass the explicit discovery/fingerprint rules.

  4. Provenance — preserve the existing canonical Co-authored-by for @THArrowofApollo through any split. Do not turn the split into "superseded, no credit" cleanup; the teardown-lock diagnosis and callback evidence remain the reason the lifecycle half exists.

Topology recommendation: keep this PR draft while extracting the lifecycle half; once both child PRs exist, close this draft as credited split/superseded-by-children, not as obsolete or duplicate. Do not merge this 23-commit branch directly into current main.

@andrexibiza andrexibiza changed the title fix(mcp-oauth): close teardown lock lifecycle draft: split MCP OAuth teardown ownership from MCP 2.0 transport composition Aug 19, 2026
@andrexibiza
andrexibiza marked this pull request as ready for review August 19, 2026 18:02
@andrexibiza andrexibiza changed the title draft: split MCP OAuth teardown ownership from MCP 2.0 transport composition split MCP OAuth teardown ownership from MCP 2.0 transport composition Aug 19, 2026

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Split execution has started on current main f43eabee5f36e11448086ee8ee17c499958e81bf: created fork branch fix/mcp-oauth-lifecycle-current-main from that exact upstream SHA. Current-main audit shows durable token invalidation has independently landed, while callback first-writer arbitration and delegated-flow owner/generation fencing have not. The lifecycle child therefore requires a residual-aware extraction, not a wholesale replay of the 23-commit branch or replacement of current mcp_oauth.py / mcp_oauth_manager.py with historical blobs. Transport-era composition remains a separate child after the lifecycle residual is isolated. @THArrowofApollo credit remains mandatory on derived publication commits.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Credited split publication receipt: lifecycle/callback child is now live as #90888 (fix/mcp-oauth-lifecycle-current-main), based on current main f43eabee5f36e11448086ee8ee17c499958e81bf, ready-for-review (draft:false). Initial exact commits: 7bc1e456d1e534ff0465da7fc9fa7c29408c99ef (owner-task delegated-flow close) and b0377b4024c9f3ac4d0864cb424174a99623ca3b (lifecycle witnesses), both carrying Co-authored-by: THArrowofApollo <igorhvr@iasylum.net>. The lifecycle child remains the place to stack callback arbitration and generation fencing. The second child remains transport/control-plane composition only and will restack on current MCP 2.0 negotiation; do not replay this parent monolithically.

Copy link
Copy Markdown
Contributor Author

Current-main transport-child audit receipt (base f43eabee5f36e11448086ee8ee17c499958e81bf): the historical transport factory should not be replayed. Current tools/mcp_tool.py already owns MCP 1.x/2.x SDK module selection, (read, write) vs legacy 3-tuple composition, server/discover vs initialize negotiation, OAuth provider reuse, SSE OAuth forwarding, TLS client factories, and SDK-native httpx/httpx2 selection.

The surviving transport/control-plane residual is narrower: _make_redirect_header_stripper() is hop-local relative to the original URL. It strips Authorization/configured headers while the next target is cross-origin, but it carries no irreversible redirect-chain taint. A chain A → B → A can therefore return to an origin comparison that looks safe even though the request crossed an untrusted origin earlier. The second credited child must preserve taint across the entire redirect chain and bind final-wire credential policy to each actual hop, without introducing another protocol-era selector, transport factory, or OAuth metadata cache.

A current-main branch now exists for that child: andrexibiza:fix/mcp2-oauth-transport-chain-taint, created from the exact same base SHA. It remains unpublished as a PR until it has a real current-main implementation + regression, rather than an empty/packet-only split.

Copy link
Copy Markdown
Contributor Author

Topology correction from current source-of-truth verification: the advertised andrexibiza:fix/mcp2-oauth-transport-chain-taint branch is not a second implementation child. Compared directly against current main (8794e5a21c980a0f26532cb4883284b786cb3f25), it is ahead_by: 0, behind_by: 46, with no changed files. It is therefore only an old main pointer and cannot be counted as publication evidence.

#90888 remains the first credited current-main child. This parent must stay open until a real second child exists for the remaining MCP 2.0 / HTTPX transport-composition residual and carries exact-head verification. Do not infer completion from the historical 23-commit train or from the empty branch.

The current-main transport already has a response-hook redirect guard (_make_redirect_header_stripper) and SDK-native sdk_httpx() composition, so the second child should be restacked from current behavior, not mechanically replay the historical transport layer. The remaining contract needs to be demonstrated by exact tests at the consuming boundary, especially OAuth control-plane client-auth preservation plus cross-origin non-restoration semantics.

Preserve @THArrowofApollo provenance while restacking the residual. I am leaving #84963 open intentionally until both children are real and independently verified.

andrexibiza added a commit to andrexibiza/hermes-agent that referenced this pull request Aug 21, 2026
Preserve the credited NousResearch#84963 lifecycle extraction and the cleanup-failure precedence hardening while composing onto current main b2c4f1f.

Refs NousResearch#38193. Preserves @THArrowofApollo / igorhvr provenance from the retained branch history.

Copy link
Copy Markdown
Contributor Author

Parent-retirement gate published in #91913

The new authority-continuity manifest records this PR as non-retirable while the MCP 2.0 transport/control-plane child is absent.

The executable retirement rule is now semantic exhaustiveness, not merely “two child PRs exist.” Before closure, every surviving parent invariant must be assigned to exactly one disposition:

  1. fix(mcp-oauth): bind delegated auth-flow teardown to the owning task #90888 OAuth lifecycle/callback ownership;
  2. the still-owed MCP 2.0 transport/control-plane child;
  3. behavior already present on current main; or
  4. explicit obsolescence under the current negotiation architecture.

The manifest also locks the negative topology: protocol-era discovery and OAuth metadata discovery remain orthogonal, and the second child may not introduce another era selector, transport factory, provider cache, or credential authority.

Contract PR: #91913
Exact head: 170a3a0e67034abd7d6a2c69a16c292b4781720f.

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 comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state 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.

5 participants