[Agent] Fix tool call budget being shared across concurrent streaming calls - #2425
Conversation
wachterjohannes
left a comment
There was a problem hiding this comment.
Thanks @ousamabenyounes, this is the right shape and it lands the fix where I could not: the budget now belongs to the call rather than to the shared Runner, and capturing it into a local before it reaches the listener closure is exactly what survives the streaming unwind.
Verified with the reproduction from #2421, unchanged:
before: stream A -> capped after 3 tool rounds
stream B -> capped after 0 tool rounds
after: stream A -> capped after 3 tool rounds
stream B -> capped after 3 tool rounds
Your new test also fails on main exactly as you describe and passes with the fix. Verified locally, all green.
On $sources: I tried to break it with two concurrent streams collecting different sources and could not either, each result keeps its own. Same output before and after this change, so leaving it in place is right.
One comment inline.
"Has the author responded?" is not answered by the issue-comment thread. A push leaves no comment at all, and a reply to an inline review comment lives on the pulls comments endpoint, not the issues one. Checking only the latter reports no activity for a PR whose author pushed a fix and replied inline, which is exactly what happened on symfony#2425. pr-review gains an explicit re-check section listing the four signals and the instruction to compare the head SHA against the reviewed commit before saying anything. pr-triage gains the consequence for bucketing: GitHub keeps CHANGES_REQUESTED until a new review is submitted, so a PR whose head moved since the last review has already been responded to. That one is actionable and belongs in the queue, not in the excluded bucket.
wachterjohannes
left a comment
There was a problem hiding this comment.
Comment reads well, and putting it on the assignment rather than above the method is the right spot, that is where someone would change it. Re-verified, all green.
Thanks for taking this on from the issue and for working out the suspension-point argument independently, that was the part that made the naive fix look correct.
fb43df6 to
1022ca9
Compare
|
Thank you @ousamabenyounes. |
"Has the author responded?" is not answered by the issue-comment thread. A push leaves no comment at all, and a reply to an inline review comment lives on the pulls comments endpoint, not the issues one. Checking only the latter reports no activity for a PR whose author pushed a fix and replied inline, which is exactly what happened on symfony#2425. pr-review gains an explicit re-check section listing the four signals and the instruction to compare the head SHA against the reviewed commit before saying anything. pr-triage gains the consequence for bucketing: GitHub keeps CHANGES_REQUESTED until a new review is submitted, so a PR whose head moved since the last review has already been responded to. That one is actionable and belongs in the queue, not in the excluded bucket.
"Has the author responded?" is not answered by the issue-comment thread. A push leaves no comment at all, and a reply to an inline review comment lives on the pulls comments endpoint, not the issues one. Checking only the latter reports no activity for a PR whose author pushed a fix and replied inline, which is exactly what happened on symfony#2425. pr-review gains an explicit re-check section listing the four signals and the instruction to compare the head SHA against the reviewed commit before saying anything. pr-triage gains the consequence for bucketing: GitHub keeps CHANGES_REQUESTED until a new review is submitted, so a PR whose head moved since the last review has already been responded to. That one is actionable and belongs in the queue, not in the excluded bucket.
Runner::$iterationslives on theRunner, which is a shared service, and is reset inrun()whennestingLevelis 0. In the streaming path the tool calling loop only starts when the returnedStreamResultis consumed, which happens much later. Two streaming calls started before either one isconsumed therefore share a single budget: the second call's reset runs while the first has not started
counting, and the first then exhausts the counter the second inherits.
With
maxToolCalls: 3, counting real tool rounds per stream:Stream B is refused before executing a single tool.
The fix
The budget now belongs to one outermost agent call instead of to the
Runnerinstance. AToolCallBudgetis created inrun()at nesting level 0, captured there, and passed into the streamlistener closure, so the loop always spends the budget of the call it belongs to — whenever the stream
is consumed.
handleToolCalls()installs the budget it was handed for the duration of the loop and restores thepreviously active one when it unwinds, so nested agent calls keep continuing on the budget of the loop
they belong to rather than opening a fresh one. That is the part the obvious "reset when nesting
level becomes 1" fix gets wrong: in the streaming path the parent's
handleToolCalls()returns beforethe nested round runs, so nesting has already unwound by the time the next round starts.
$sourcesand$nestingLevelare deliberately left on theRunnerfor now:$nestingLevelcorrectlytracks the synchronous call stack (
handleToolCalls()has no suspension point, so it always runs tocompletion once the listener fires), and I could not construct a failing case for
$sourcesunderconcurrent streams — each level already attaches and resets it before unwinding. Happy to move them
into the same per-call object if you prefer that as one deliberate change.
Test verification (RED → GREEN)
RED — new test on unmodified
main, production code untouched (git status --porcelain -- src/agent/src/empty):The first assertion passes and the second fails at
3, which is exactly the reported behaviour: thefirst stream spends its 3 rounds, the second gets 0.
GREEN — same test with the fix:
No regressions in the component —
mainwasOK (276 tests, 656 assertions), the branch is:php-cs-fixerreports 0 fixable files andphpstan analysereports no errors forsrc/agent.