fix(mcp): abandon open SSE without poisoning pooled HTTP sockets

[ningyixi]

Summary

Fixes #1079.

RubyLLM::MCP::HTTP::Stream#feed stopped reading an open chunked SSE body with throw as soon as the JSON-RPC answer arrived. throw is not an Exception, so Net::HTTP::Persistent returned that socket to its pool with the chunked terminator (0\r\n\r\n) still unread. The next request on the same thread then failed with:

Faraday::ConnectionFailed: wrong status line: "0"

Fix

• Raise Stream::Abandoned when the answer arrives instead of throw.
• Rescue it in Stream#read and treat it as a normal early stop.
• Adapters that rescue Exception (including net_http_persistent) then finish the connection so it is not reused dirty.
• Default :net_http behavior is unchanged; existing open-stream specs still pass.

Verification

bundle exec rspec spec/ruby_llm/mcp/http_spec.rb
# 54 examples, 0 failures

New example: two successive tools/call requests against a loopback server that keeps the SSE stream open after the answer, with config.faraday_adapter = :net_http_persistent. Before the fix the second call failed with wrong status line: "0"; after, both succeed.

Also added faraday-net_http_persistent as a development dependency so that regression can run in CI.

Summary by CodeRabbit

• Bug Fixes
  • Streaming requests now stop reading once a response is received, even if the event stream remains open.
  • Follow-up requests can proceed reliably when using persistent connections after a streaming response.
  • Connections with unread streaming data are handled safely, helping prevent lingering streams from interfering with subsequent requests.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: crmne/ruby_llm/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: a2371495-ec55-422a-91c6-bd42638efdb8

📥 Commits

Reviewing files that changed from the base of the PR and between a17691f and ca6a397.

📒 Files selected for processing: 5

• Gemfile
• gemfiles/rails_7.1.gemfile
• gemfiles/rails_7.2.gemfile
• gemfiles/rails_8.0.gemfile
• gemfiles/rails_8.1.gemfile

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

The HTTP stream now raises and rescues Abandoned when an answer arrives. A real-connection test checks consecutive tools/call requests with the net_http_persistent adapter.

Changes: MCP HTTP stream handling

Layer / File(s)	Summary
Answer-triggered stream termination lib/ruby_llm/mcp/http.rb	Stream#feed raises Abandoned when an answer is available. read rescues the exception and returns the stream.
Persistent adapter regression test Gemfile, gemfiles/rails_7.1.gemfile, gemfiles/rails_7.2.gemfile, gemfiles/rails_8.0.gemfile, gemfiles/rails_8.1.gemfile, spec/ruby_llm/mcp/http_spec.rb	Development dependencies add faraday-net_http_persistent and net-http-persistent; the Rails Gemfiles also add MRI-only mxc. A real-connection test checks that two consecutive tools/call requests succeed when the event streams remain open.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: crmne

Merge Risk: ⚪ Minimal · up to ca6a3

The answer-triggered stream stop allows the pooled adapter to finish the connection before the next request. No actionable merge-blocking risk remains.

### Architecture Summary

Architecture risk: 🔵 Low · up to ca6a3

The change affects 4 systems.

Changed systems: gemfiles, Gemfile, lib, spec

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — gemfiles (service) was modified; 4 changed files map to changed impact.
• observed — Gemfile (service) was modified; 1 changed file maps to changed impact.
• observed — lib (service) was modified; 1 changed file maps to changed impact.
• observed — spec (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

• observed — Modified behavior in lib/ruby_llm/mcp/http.rb: The comments now describe answer-triggered stream termination as a raise that lets pooled adapters discard connections with unread bytes, and say a stream breaking midway counts as ended. Stream adds the Abandoned exception used for that termination.
• observed — Modified behavior in lib/ruby_llm/mcp/http.rb: read now yields directly and rescues Abandoned to return the stream; it no longer catches a throw targeted at the stream object. Header capture and the subsequent Faraday::ConnectionFailed handling remain.
• observed — Modified behavior in lib/ruby_llm/mcp/http.rb: When an answer is available, feed now raises Abandoned instead of throwing to the stream object.
• observed — Modified behavior in spec/ruby_llm/mcp/http_spec.rb: Adds a real-connection test that selects :net_http_persistent, serves two event streams that remain open after sending answers, and checks that two consecutive tools/call requests succeed. The test requires the persistent adapter and resets the configured adapter to :net_http in ensure.

Pre-merge checks: 3 · 2

Failed checks: 2 warnings

Check name	Status	Explanation	Resolution
Description check		The description clearly explains the problem, fix, affected adapter behavior, regression test, and reported test results. However, it does not follow the repository template and omits the Type of chan…	Update the description to include all template sections and mark the applicable checklist items, especially Bug fix, testing and hook results, scope confirmation, ownership, and No API changes.
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. (5 skipped: 5 …	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 3 passed

Check name	Status	Explanation
Title check		The title clearly and concisely describes the main change: abandoning open SSE streams without corrupting pooled HTTP connections.
Linked Issues check		Issue [#1079] requires safe abandonment of an answered SSE stream before pooled connection reuse. Stream#feed now raises Stream::Abandoned, and Stream#read handles it as a normal early stop afte…
Out of Scope Changes check		The production change directly fixes the pooled-connection failure in [#1079]. The regression test exercises the reported failure mode. The Gemfile and appraisal Gemfile updates provide the adapter de…

Full details: Description check

Explanation

The description clearly explains the problem, fix, affected adapter behavior, regression test, and reported test results. However, it does not follow the repository template and omits the Type of change, Scope check, Quality check, Ownership, and API changes sections.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. (5 skipped: 5 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[juneira]

The library change looks right. I verified the mechanism independently: with throw, Net::HTTP::Persistent#request's rescue Exception (which calls finish connection) never runs, so the socket goes back to the pool with the chunked terminator still unread; with raise, the connection is finished and the next request reconnects cleanly. Reproduced against a loopback server that answers inside an open chunked SSE stream: throw fails the second request with wrong status line: "0", raise succeeds on both.

One fix needed for CI: the new regression spec breaks the Faraday v1 job (https://github.com/crmne/ruby_llm/actions/runs/38645047543/job/116129707529). With FARADAY_VERSION=1.10.3, Bundler resolves faraday-net_http_persistent to 1.2.0, which does not depend on net-http-persistent. That gem is not in the v1 bundle, so require 'faraday/net_http_persistent' succeeds (it only registers the adapter), and the failure surfaces later, when Faraday 1's dependency loader instantiates the adapter and tries to require 'net/http/persistent':

RuntimeError:
  missing dependency for Faraday::Adapter::NetHttpPersistent:
  cannot load such file -- net/http/persistent

Suggested guard:

it 'reuses a pooled adapter after abandoning an open event stream' do
  begin
    require 'faraday/net_http_persistent'
    require 'net/http/persistent'
  rescue LoadError
    skip 'net_http_persistent is not available'
  end
  ...

The Faraday 2 jobs keep running the example; only the v1 job, whose bundle cannot load the adapter's real dependency, skips it. The fix itself is Faraday-version independent, since the rescue Exception that finishes the connection lives in net-http-persistent, not in the adapter.

[github-actions]

Please guard the regression example for environments where the persistent adapter's runtime dependency is unavailable. Requiring both faraday/net_http_persistent and net/http/persistent, then skipping on LoadError, keeps the Faraday v1 job green while retaining coverage in Faraday 2.

Generated by Copilot Triage using openai/gpt-5.6-luna; 17933 input / 475 output tokens ($0.0021) this run; view run.