fix: propagate contextvars to tool executor threads#6854
Conversation
run_in_executor does not propagate contextvars to worker threads, causing execution context params like data_dir to be lost when MCP tools are called. This made save_data, serve_file_to_user, and other tools that depend on auto-injected data_dir fail with "Missing required argument: data_dir". Fix: use contextvars.copy_context().run() to carry the current context into the thread pool worker.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR ensures contextvars set in the async execution context (e.g., Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/tests/test_tool_context_propagation.py (1)
16-17: Prefer publicToolRegistrycontext helpers in test setup/teardown.Lines 45-55 directly mutate private
_execution_context; usingToolRegistry.set_execution_context/reset_execution_contextbetter matches production flow and reduces coupling.💡 Proposed refactor
-from framework.runner.tool_registry import _execution_context +from framework.runner.tool_registry import ToolRegistry, _execution_context @@ - token = _execution_context.set({"data_dir": "/tmp/test_data"}) + token = ToolRegistry.set_execution_context(data_dir="/tmp/test_data") @@ - _execution_context.reset(token) + ToolRegistry.reset_execution_context(token)Also applies to: 45-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/tests/test_tool_context_propagation.py` around lines 16 - 17, Tests are directly importing and mutating the private _execution_context; replace those direct mutations with the public API by importing ToolRegistry and calling ToolRegistry.set_execution_context(...) in the setup and ToolRegistry.reset_execution_context() in teardown (or the equivalent methods on ToolRegistry) so the test uses the same context management as production and avoids touching _execution_context directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/tests/test_tool_context_propagation.py`:
- Line 12: Add "from __future__ import annotations" at the top of
core/tests/test_tool_context_propagation.py and add explicit return type
annotations (-> None) to the test function signatures mentioned in the review
(the test functions defined at the commented lines 22, 29, and 65, e.g., the
test_* functions that exercise tool context propagation). Ensure each function
signature includes the return type annotation (-> None) and keep existing
parameter types/annotations intact so the file complies with the repository
typing rules.
---
Nitpick comments:
In `@core/tests/test_tool_context_propagation.py`:
- Around line 16-17: Tests are directly importing and mutating the private
_execution_context; replace those direct mutations with the public API by
importing ToolRegistry and calling ToolRegistry.set_execution_context(...) in
the setup and ToolRegistry.reset_execution_context() in teardown (or the
equivalent methods on ToolRegistry) so the test uses the same context management
as production and avoids touching _execution_context directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01997f23-f525-4ced-abbb-171c32a53ab3
📒 Files selected for processing (2)
core/framework/graph/event_loop/tool_result_handler.pycore/tests/test_tool_context_propagation.py
Verifies that execution context (data_dir, etc.) set via set_execution_context is visible inside tool executors that run in thread pool workers via run_in_executor.
4e2c0fe to
51344fc
Compare
* fix: propagate contextvars to tool executor threads run_in_executor does not propagate contextvars to worker threads, causing execution context params like data_dir to be lost when MCP tools are called. This made save_data, serve_file_to_user, and other tools that depend on auto-injected data_dir fail with "Missing required argument: data_dir". Fix: use contextvars.copy_context().run() to carry the current context into the thread pool worker. * test: regression test for contextvars propagation in tool executor Verifies that execution context (data_dir, etc.) set via set_execution_context is visible inside tool executors that run in thread pool workers via run_in_executor.
Description
Fix MCP tool context params (data_dir, etc.) being silently lost when tool executors run in thread pool workers, causing save_data, serve_file_to_user, and other data tools to fail with "Missing required argument: data_dir".
Type of Change
Related Issues
Fixes #6853
Changes Made
core/framework/graph/event_loop/tool_result_handler.py: Wraptool_executorcall incontextvars.copy_context().run()so execution context (data_dir, etc.) propagates into worker threads spawned byrun_in_executorcore/tests/test_tool_context_propagation.py: Regression test verifying contextvars are visible inside tool executors running in thread poolTesting
cd core && pytest tests/)cd core && ruff check .)Checklist
Summary by CodeRabbit
Bug Fixes
Tests