Skip to content

Fix: concurrent tool execution fails with ProcessDriver due to $this capture and circular references#1009

Open
vinitkadam03 wants to merge 4 commits intoprism-php:mainfrom
vinitkadam03:fix/concurrency-serializable-closure
Open

Fix: concurrent tool execution fails with ProcessDriver due to $this capture and circular references#1009
vinitkadam03 wants to merge 4 commits intoprism-php:mainfrom
vinitkadam03:fix/concurrency-serializable-closure

Conversation

@vinitkadam03
Copy link
Copy Markdown
Contributor

Summary

Fixes #893 — concurrent tool execution via Laravel's Concurrency::run() fails because closures capture $this (the handler object), causing SerializableClosure to serialize the entire application container. This also fixes the ->using($this) circular reference issue for class-based invokable tools.

Changes

CallsTools trait — eliminate $this capture in concurrent closures

  • Replaced groupToolCallsByConcurrency with resolveToolCalls which resolves each tool exactly once upfront and returns {tool, toolCall} pairs. Unresolvable tools carry the caught exception forward instead of being re-resolved inside the closure.
  • Replaced instance method executeToolCall with static method executeResolvedToolCall that receives the pre-resolved Tool directly — no $this, no $tools array in the serialization boundary.
  • Concurrent closures are now static fn() calling self::executeResolvedToolCall(), preventing the handler (and its transitive references to the Laravel container) from being serialized.

Tool class — fix circular reference and named argument handling

  • using() now detects $fn === $this and skips the assignment, preventing the Tool -> $fn -> Tool circular reference that breaks SerializableClosure.
  • handle() auto-detects __invoke() on subclasses when $fn is null, so invokable tool classes work with or without ->using($this).
  • handle() unwraps SerializableClosure\Serializers\Native wrappers via getClosure() before invocation, restoring PHP 8 named argument forwarding that Native::__invoke() breaks.

Fixes

  • TypeError: Cannot assign SerializableClosure\Serializers\Native to property ... (e.g. Scramble's GeneratorConfig::$uiRoute)
  • Error: Cannot modify readonly property ... (e.g. Spatie LaravelData's DataType::$type)
  • Named arguments silently broken after ProcessDriver deserialization
  • Tool -> $fn -> Tool circular reference with class-based invokable tools using ->using($this)

Test plan

  • Existing CallsToolsConcurrentTest — sequential, concurrent, mixed, error handling, grouping
  • Existing CallsToolsTest — all tool execution and event generation paths
  • Existing ToolTest and ToolErrorHandlingTest — all tool behavior
  • Existing ConcurrentToolExecutionTest — concurrent flag behavior
  • New: invokable subclass with ->using($this) works without circular reference
  • New: invokable subclass works without calling using() at all

- Updated `CallsTools` trait to rename and enhance the `groupToolCallsByConcurrency` method to `resolveToolCalls`, which now returns structured pairs of tools and tool calls.
- Modified the execution logic to handle resolved tool calls, ensuring proper error handling for unresolvable tools.
- Adjusted the `handle` method in `Tool` class to correctly unwrap closures for named argument support.
- Updated tests to reflect changes in method names and structure.
…tests

- Updated the `Tool` class to initialize the `$fn` property as `null` by default.
- Added a check in the `using` method to return `$this` if the provided function is the instance itself, preventing circular references.
- Improved the `handle` method to throw an exception if no valid function is defined.
- Introduced new tests for handling invokable subclasses, ensuring they work correctly with and without explicit function assignments.
Tool::handle() now auto-detects __invoke() on subclasses, making
the explicit ->using($this) call redundant. Removing it also
eliminates the circular reference that broke SerializableClosure.
- Introduced a new method `resolveHandler()` to encapsulate the logic for determining the callable handler for the tool.
- Simplified the `handle()` method by delegating handler resolution to `resolveHandler()`, improving readability and maintainability.
- Enhanced error handling for cases where no valid handler is defined, ensuring consistent behavior across invokable subclasses.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Concurrent tool execution fails with Laravel's ProcessDriver due to circular reference and $this capture in closures

1 participant