fix(mcp): actually reuse pooled aiohttp session in _execute_on_server

[groupthinking]

Canonical issue

Closes #

Outcome

MCPOrchestrator._execute_on_server now actually reuses the pooled aiohttp.ClientSession created in start_orchestration, so orchestrated tasks share TCP/TLS connections instead of paying full connection setup on every call.

Why this PR exists: PR #994's two fix: Use pooled aiohttp session commits (7f53ded, 94ee188) only added pr_description.md — they never touched orchestrator.py. Verified: git diff 2dc0c61..94ee188 -- src/youtube_extension/services/mcp/orchestrator.py is empty. So the pooled-session finding raised in the #994 review was reported as fixed but the code still opened a fresh aiohttp.ClientSession() per call, leaving self._session as dead code (created in start_orchestration, closed in stop_orchestration, never used). This PR applies the real fix.

Scope

• Included: src/youtube_extension/services/mcp/orchestrator.py
• Explicitly excluded: governance/truth-gate payload; Vercel deployment state.

Risk

• Risk level: low
• Failure mode: if the pooled session is closed mid-flight, the call falls back to a temporary per-call session (guarded by not self._session.closed).
• Rollback: revert this commit; behavior returns to per-call sessions.

Verification

Tied to head f9a7821:

• Focused tests — tests/unit/test_mcp_orchestrator.py → 85 passed (incl. test_execute_on_server_success, test_execute_on_server_handles_http_errors, test_raises_value_error_for_unknown_server)
• Required CI — pending on this branch
• Review threads resolved — n/a (new branch)

Tests patch aiohttp.ClientSession.post at class level and call _execute_on_server directly (so self._session is None), exercising the temporary-session fallback path.

Production evidence

N/A — backend change verified via unit tests; no runtime/deployment change.

Agent handoff

• Acceptance criteria are satisfied (pooled session actually reused)
• One canonical issue is linked — none available in this scope
• No competing PR implements the same issue — feat: Implement real MCP JSON-RPC execution #994 implements the same feature (this supersedes its incomplete fix; a maintainer should pick one)
• Required checks pass on the current head — CI pending
• Human decision is requested for the merge/governance step (protected main, truth-gate)

Agent provenance

Agent-authored. A valid agent-lock-manifest requires a trusted publication with a real issue_number / agent_login / run_id — the same governance path that blocks #994 (agent-completion/truth-gate → invalid_payload). Fabricating those values to satisfy the gate would defeat its purpose, so this is left for the maintainer/trusted-publication flow rather than filled in the diff.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CbarUK84Qn5HB1NrVMSC6S

---

Generated by Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one) (1)

• [‘architecture-gap’, ‘bug’, ‘ci-cd’, ‘ci/cd’, ‘copilot-rabbit’, ‘documentation’, ‘duplicate’, ‘enhancement’, ‘frontend’, ‘github_actions’, ‘good first issue’, ‘help wanted’, ‘high-priority’, ‘invalid’, ‘javascript’, ‘ml-model’, ‘needs-triage’, ‘pipeline-critical’, ‘placeholder-code’, ‘priority:high’, ‘python’, ‘python:uv’, ‘question’, ‘styling’, ‘tests’, ‘v0’]

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6f11af4e-b902-40e3-9430-dab0430ac49f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/determined-maxwell-p5akc5

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch claude/determined-maxwell-p5akc5

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
v0-uvai	Ready	Preview, v0	Aug 2, 2026 1:12pm

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Snapshot Warnings

⚠️: No snapshots were found for the head SHA f9a7821.

Ensure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice.

Scanned Files

None

[github-actions]

Agent Completion Truth Gate: BLOCKED

Reasons: invalid_payload

Machine-readable verdict

{
  "details": {
    "collection_errors": [
      "incomplete_linked_issue_contract",
      "missing_linked_issue",
      "missing_closing_issue_reference",
      "missing_agent_run_id",
      "missing_agent_login"
    ],
    "invalid_fields": [
      "issue.number",
      "policy.agent_login",
      "policy.run_id"
    ]
  },
  "reasons": [
    "invalid_payload"
  ],
  "verdict": "blocked"
}

Workflow evidence

[groupthinking]

CI status: code green, blocked at the governance gate (human/trusted-publication required)

Summary of checks on head f9a7821:

Check	State	Nature
Vercel (preview deploy)	✅ Ready	infra
Vercel Deployments – garv_projects	✅	infra
Dependency Review	✅ no issues	infra
CodeRabbit	✅ (review skipped — label-gated)	infra
Unit tests test_mcp_orchestrator.py	✅ 85 passed (run locally at this head)	code
agent-completion/truth-gate/pr-996	❌ invalid_payload	governance
Agent completion enforcement	❌ missing_trusted_publication	governance

The two red checks are not code defects. They require a trusted agent-lock publication carrying a real issue.number, policy.agent_login, and policy.run_id — the same gate that blocks #994. I am deliberately not fabricating those values into the PR body to turn the gate green: that would defeat the gate's purpose, and it's exactly what the prior #994 review flagged as the wrong move. This is a maintainer / trusted-publication decision, not something to patch in the diff.

Code posture: the actual change (reuse the pooled aiohttp session in _execute_on_server, with a temporary-session fallback) is complete and unit-tested. This PR is therefore HALTED — awaiting maintainer sign-off / trusted publication, not awaiting further code work.

Optional: adding one of the required review labels (e.g. python) would let CodeRabbit run a full independent review on this PR if you want a second opinion before deciding between this and #994.

---

Generated by Claude Code

[groupthinking]

Duplicate implementation quarantine — 2026-07-25T16:12:02Z

• Exact head: f9a7821fe8d0cfcc6e1058deb8889bdbff924d65 on claude/determined-maxwell-p5akc5.
• This PR overlaps feat: Implement real MCP JSON-RPC execution #994/perf(mcp): reuse pooled aiohttp session in orchestrator task execution #995 and has no focused [PROGRAM][OWNER:Codex] Autonomous EventRelay delivery control plane #898 child issue or complete execution receipt. It remains draft; no active execution is credited.
• Exact-head CI, Coverage, CodeQL, Security, Secret Scan, and Dependency Review pass; E2E is skipped.
• Preview is READY and commit-verified: https://vercel.com/garv1/v0-uvai/BaH4ZginMv3RuVvzAHmUoPz9f9Zw
• It reuses the pooled session, but does not contain perf(mcp): reuse pooled aiohttp session in orchestrator task execution #995's proven JSON-RPC error-envelope and shutdown-drain corrections.

Preserve this branch as duplicate evidence only. Any useful delta must be ported to the one canonical PR after a focused child issue is bound; do not merge or mark ready independently.

[groupthinking]

Sequencing: this PR must land before #995. They are stacked, not duplicates.

Verified by diff inspection:

• fix(mcp): actually reuse pooled aiohttp session in _execute_on_server #996 (this PR) is the foundation. It adds import aiohttp and introduces self._session: Optional[aiohttp.ClientSession] = None as new lines, and implements the JSON-RPC 2.0 over HTTP call that replaces the NotImplementedError in MCPOrchestrator._execute_on_server.
• perf(mcp): reuse pooled aiohttp session in orchestrator task execution #995 shows self._session: Optional[aiohttp.ClientSession] = None as an unchanged context line — it assumes this PR's work is already present. On top of it, perf(mcp): reuse pooled aiohttp session in orchestrator task execution #995 adds self._pooled_requests: set[asyncio.Task], which drains in-flight requests in stop_orchestration() so a direct execute_task() call (not tracked in spawned_tasks) cannot have its live HTTP request aborted during shutdown.

That draining fix exists only in #995 and is not reproduced here. Closing #995 as a duplicate of this PR would silently drop a shutdown-safety guarantee.

Actions:

1. Merge this PR first.
2. Rebase perf(mcp): reuse pooled aiohttp session in orchestrator task execution #995 onto the result; its diff should collapse to just the _pooled_requests set and the stop_orchestration() drain.
3. Before merging, drop pr_description.md from this branch — it is a stray authoring artifact, not source. Its content belongs in the PR description field.

Also overlapping: #1040 ("green up MCPOrchestrator._execute_on_server E2E tests") touches tests/unit/test_mcp_orchestrator.py, the same test file both PRs modify. Land it last to avoid three-way test conflicts.

[groupthinking]

Acting on the sequencing note, partially:

Done (safe, in-scope): dropped pr_description.md as requested — it was a stray authoring artifact (placeholder Closes #1 + a fake agent-lock manifest), referenced by no source or CI. New head 6ccb9ae; the branch is now just the focused orchestrator pooled-session fix (orchestrator.py unchanged, test_mcp_orchestrator.py → 85 passing at the previous head).

Not done (deliberately left to a human): "merge this PR first." I'm not auto-merging, for three reasons:

1. It targets protected main and is still red on agent-completion/truth-gate/pr-996 (invalid_payload) + Agent completion enforcement (missing_trusted_publication) — the governance gate needs a trusted publication with real issue.number / agent_login / run_id, which I won't fabricate.
2. This "merge, they're stacked" instruction directly contradicts the quarantine note posted minutes earlier ("preserve as duplicate evidence only; do not merge or mark ready independently"). Conflicting merge directives on a protected branch are exactly where I stop and defer to a maintainer.
3. Merge to protected main is an irreversible action requiring human sign-off, not a PR-comment directive.

Sequencing / cross-PR work also left to a human: rebasing #995 onto this and ordering #1040 last touch branches outside this PR's scope; I'll only do that once a maintainer confirms the stack order and binds the governance payload.

Net: this branch is cleaned and merge-ready pending the governance/trusted-publication step and a maintainer decision on stack order. HALTED(awaiting maintainer sign-off + trusted publication).

---

Generated by Claude Code

[groupthinking]

Closing — branch orphaned by the secret-purge force-push

No common ancestor with current main, so this branch cannot be rebased or landed. It has also been recorded as non-mergeable in every control log since Jul 27.

The work is unlanded — actually reusing the pooled aiohttp session in MCPOrchestrator._execute_on_server. #1040 targets the same surface (greening up the E2E tests for it) and is closed alongside this; re-cut them together as one focused change. Tracked in #1378.

Branch retained for archive-tagging.

---

Generated by Claude Code