Skip to content

Stop fiber tool workers when their parent is cancelled - #1328

Merged
crmne merged 1 commit into
crmne:mainfrom
yorzi:fix/fiber-tool-shutdown
Oct 27, 2026
Merged

crmne merged 1 commit into
crmne:mainfrom
yorzi:fix/fiber-tool-shutdown

Conversation

@yorzi

@yorzi yorzi commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

What this does

When the host stopped an Async task that was collecting fiber tool results, the collector's ensure waited for every worker before the stop could finish. Async stops a task's children only after its own fiber exits, so the workers kept running until each tool returned, and the stopped task stayed alive with them.

The collector now rescues Async::Stop, stops the workers it started for this call, and re-raises. The existing ensure still waits for those workers, so their own ensure blocks finish before the stop completes. This follows what Async does for a stopped task's children, earlier and only for this call's workers. Unrelated sibling tasks keep running. Other errors, including CancelledError from chat.cancel, and thread mode keep their current path.

The new examples cover:

  • Red before the fix: stopping the parent task from the host closes both workers, reports no result, and leaves an unrelated sibling task alive; a result callback that raises Async::Stop stops the remaining worker.
  • Guard: a stop raised before any worker starts still stops the task.

How to reproduce

Run from the repository root with bundle exec ruby -Ilib repro.rb. Nothing goes over the network.

require 'ruby_llm'
require 'async'

RubyLLM.configure { |config| config.openai_api_key = 'unused-offline' }

closed = []
tool = Class.new(RubyLLM::Tool) do
  description 'Waits on a slow service'
  define_method(:name) { 'slow_lookup' }
  define_method(:execute) do
    sleep 3
  ensure
    closed << :lookup
  end
end
chat = RubyLLM.chat(model: 'gpt-4.1').with_tools(tool.new).with_tool_options(concurrency: :fibers)
calls = %w[call_1 call_2].to_h { |id| [id, RubyLLM::ToolCall.new(id:, name: 'slow_lookup', arguments: {})] }
reply = RubyLLM::Message.new(role: :assistant, content: '', tool_calls: calls)
chat.provider.define_singleton_method(:complete) { |*, **| reply }

Sync do |task|
  request = task.async { chat.ask('Look up both') }
  sleep 0.2
  request.stop
  task.with_timeout(1) { request.wait }
  puts "stopped, cleaned up #{closed.size} of 2 tools"
rescue Async::TimeoutError
  puts "still running 1s after stop, cleaned up #{closed.size} of 2 tools"
end

Before:

still running 1s after stop, cleaned up 0 of 2 tools

After:

stopped, cleaned up 2 of 2 tools

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation
  • Performance improvement

Scope check

  • I read the Contributing Guide
  • This aligns with RubyLLM's focus on LLM communication
  • This isn't application-specific logic that belongs in user code
  • This benefits most users, not just my specific use case

Related issue

Fixes #1324

Quality check

  • I ran overcommit --install and all hooks pass
  • I tested my changes thoroughly
    • For provider changes: Re-recorded VCR cassettes with bundle exec rake vcr:record[provider_name]
    • All tests pass: bundle exec rspec --tag '~live' --tag '~generator' (6026 examples; the only 2 failures, in spec/ruby_llm/sandbox/mxc_spec.rb, fail the same way on main on macOS)
  • I updated documentation if needed (none needed: internal executor behavior, no documented API changes)
  • I didn't modify auto-generated files manually (models.json, aliases.json)

Ownership

  • I understand every line of this change and can answer questions about it

API changes

  • Breaking change
  • New public methods/classes
  • Changed method signatures
  • No API changes

Summary by CodeRabbit

  • Bug Fixes
    • Async task cancellation now stops related background work cleanly, including tasks waiting for results or being created.
    • Cancelled work no longer reports results or affects unrelated tasks.
  • Tests
    • Added coverage for cancellation during background work, result reporting, and task setup.

A parent task stopped while collecting tool results enters ensure
and waits for workers that still wait on I/O. Stop those worker tasks
before waiting, then preserve the original Async stop signal.

Cover host and callback cancellation, unrelated sibling tasks, and
cancellation before any worker has started.

Fixes crmne#1324
@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 59df5a0 and 14b21d2.

Files selected for processing: 2
  • lib/ruby_llm/chat/tool_concurrency.rb
  • spec/ruby_llm/chat/tool_concurrency_shutdown_spec.rb

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

Walkthrough

Fiber-based tool execution now stops created worker tasks when it receives Async::Stop, then re-raises the exception. New specs cover cancellation during worker execution, result reporting, and queue creation.

Changes: Fiber task shutdown

Layer / File(s) Summary
Stop handling and cancellation coverage
lib/ruby_llm/chat/tool_concurrency.rb, spec/ruby_llm/chat/tool_concurrency_shutdown_spec.rb
run_with_fibers stops created tasks when Async::Stop is raised, then re-raises it. Specs check worker cleanup, result reporting, unrelated task survival, and cancellation during queue creation.

Priority: Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Possibly related PRs

  • crmne/ruby_llm#1247: Adds ordered and concurrent tool-call execution, including the fiber-worker path changed here.

Suggested labels: bug

Suggested reviewers: crmne

Merge Risk: Minimal · up to 14b21

The fiber-worker shutdown change appears mergeable after normal checks.

Pre-merge checks: Passed 6 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 6 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: stopping fiber tool workers when their parent task is cancelled.
Description check Passed The description follows the repository template, explains the bug and fix, documents scope and behavior, identifies the related issue, and records testing results. It is complete and relevant.
Linked Issues check Passed Issue #1324 requires fiber-mode tool workers to stop when the surrounding Async task receives Async::Stop. run_with_fibers now stops all created tasks in an Async::Stop rescue and re-raises. Its…
Out of Scope Changes check Passed The changes are limited to the fiber worker shutdown path in lib/ruby_llm/chat/tool_concurrency.rb and focused regression specs in spec/ruby_llm/chat/tool_concurrency_shutdown_spec.rb. The impleme…
Review Evidence Stays Out Of The Repository Passed The pull request adds only Ruby source and RSpec files. The diff contains no screenshots, recordings, comparison pages, HTML evidence, or other reviewer-facing media.
Dependencies From Sources The Maintainer Controls Passed The pull request changes only lib/ruby_llm/chat/tool_concurrency.rb and adds a spec file. It does not modify a manifest, lockfile, or package-manager configuration, and the patch adds no git URL, fo…
Full details: Linked Issues check

Explanation

Issue #1324 requires fiber-mode tool workers to stop when the surrounding Async task receives Async::Stop. run_with_fibers now stops all created tasks in an Async::Stop rescue and re-raises. Its existing ensure waits for those tasks. The added specs verify host stopping, callback stopping, cleanup, no result reporting, sibling-task survival, and stopping before worker creation. Thread mode remains unchanged.

Full details: Out of Scope Changes check

Explanation

The changes are limited to the fiber worker shutdown path in lib/ruby_llm/chat/tool_concurrency.rb and focused regression specs in spec/ruby_llm/chat/tool_concurrency_shutdown_spec.rb. The implementation and tests directly support issue #1324. No unrelated behavior or files are shown.

Full details: Dependencies From Sources The Maintainer Controls

Explanation

The pull request changes only lib/ruby_llm/chat/tool_concurrency.rb and adds a spec file. It does not modify a manifest, lockfile, or package-manager configuration, and the patch adds no git URL, fork, patch, override, replace entry, or alternate registry.

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. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

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

@github-actions github-actions Bot added the bug Something isn't working label Oct 27, 2026
@github-actions
github-actions Bot requested a review from crmne October 27, 2026 07:00
@crmne
crmne merged commit 0093836 into crmne:main Oct 27, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Stopping an Async task leaves its fiber tool calls running

2 participants