[Feature] Parallel Tool Call Execution - #926
Conversation
weiqingy
left a comment
There was a problem hiding this comment.
Thanks for pushing this through. A few questions inline.
| * failed; slots that already completed keep their success or failure outcome. | ||
| */ | ||
| public static final ConfigOption<Duration> TOOL_CALL_BATCH_TIMEOUT = | ||
| new ConfigOption<>("tool-call.batch.timeout", Duration.class, Duration.ofMillis(-1)); |
There was a problem hiding this comment.
This is the repo's first ConfigOption<Duration>, and I do not think it can be set from any documented route.
AgentConfiguration.get() dispatches isAssignableFrom, String, Integer, Long, Float, Double, Boolean, isEnum(), then throw new ClassCastException. There is no Duration branch, and YAML values arrive as String or Integer. So getConfig().get(TOOL_CALL_BATCH_TIMEOUT) at JavaRunnerContextImpl.java:253 throws for any value a user sets, after :119 has already persisted N PENDING slots, and the catch-all at ToolCallAction.java:171 then reports every tool in the batch failed.
The programmatic route clears get() but not plan serialization: AgentPlan.writeObject uses a bare new ObjectMapper() with no JavaTimeModule, and I reproduced InvalidDefinitionException on java.time.Duration against the pinned jackson-databind. On Python, core_options.py:271 is config_type=int, so the documented 30s raises ValueError mid-action.
CI stays green because JavaRunnerContextImplDurableExecuteAsyncTest.java:324-327 sets the option in-process, where isAssignableFrom short-circuits. And check_java_python_config_options_parity.py:45 adds "java.time.Duration": int to the type table, so the harness now certifies exactly this pair.
ConfigurationUtils.convertValue is already imported in AgentConfiguration for the enum branch and it handles Duration; retyping this option to Long millis would instead let the harness stay strict. Do you have a preference? Asking because the batch deadline is the only bound on a hung tool wedging the fan-in, and it currently ships both default-off and unsettable.
There was a problem hiding this comment.
Thanks for the catch. I missed the YAML case and have now updated the timeout configuration to use the Long type
There was a problem hiding this comment.
The type change buys more than the YAML fix, for what it's worth — AgentConfiguration.get() has a Long branch at AgentConfiguration.java:142, and Long is Jackson-native, so the plan-serialization and Python routes resolve along with it.
Two smaller things while the option is in flux. short-term-memory.state-ttl.ms (AgentExecutionOptions.java:88) is the existing Long-millis option in this class and puts the unit in the key — since a bare Long drops the unit Duration carried, would tool-call.batch.timeout.ms be worth matching to it while the key is still unreleased?
And a few Duration leftovers will probably want sweeping with the same change: the parity-harness entries added for it (check_java_python_config_options_parity.py:45 and the _java_duration_to_millis branch at :99) are covered by "java.lang.Long": int at :40 once the type moves, and the config table still lists the type as Duration with a -1ms default (configuration.md:137). Any reason to keep those around once the type moves?
| return outcomes; | ||
| } | ||
|
|
||
| private <T> List<Outcome<T>> collectTimedOutOutcomes(List<Callable<T>> suppliers) { |
There was a problem hiding this comment.
testToolCallBatchExecutionIsActuallyParallel covers the real java21 path well, fallback branch included, which matters because surefire runs the exploded target/classes and so never loads the META-INF/versions/21 classes. The deadline leg looks like the remaining gap, and this stub is what makes it look covered.
collectTimedOutOutcomes hard-codes "index 0 succeeds, everything else is a TimeoutException" and never reads the timeout argument. So in testDurableExecuteAllAsyncTimeoutKeepsCompletedOutcomes the 10 ms deadline set at :324-327 and the Thread.sleep(100) in the second supplier are both inert, supplier 1 is never invoked, and its getCallCount() is never asserted. A deadline computed with the wrong sign, a cancel on the wrong future, or a barrier that never resolves would all still pass. What it does cover, a timed-out slot persisted FAILED with the cursor still advancing by 2, is worth keeping.
Now that the e2e path exists, would a batch that overruns a short tool-call.batch.timeout be the cheapest way to get getDeadlineNanos and collectBatchOutcomesOnTimeout genuinely exercised? That may have to wait on the Duration conversion on AgentExecutionOptions.java:82, since setting the option is what trips the plan serializer.
There was a problem hiding this comment.
I will add e2e tests to cover realistic timeout scenarios after the previous configuration issue is fixed.
There was a problem hiding this comment.
Sounds good, and sequencing it after the config fix makes sense. One gap the e2e won't close though: the stub at :404 ignores the timeout argument it's handed, so that assertion passes regardless of the deadline. Worth tightening at the same time?
| callables.subList(plan.executionStart, callables.size())) { | ||
| ids.add(callable.getId()); | ||
| } | ||
| reservePendingBatch(ids, argsDigest); |
There was a problem hiding this comment.
Reserving PENDING for every call in the batch, non-reconcilable ones included, is the intended shape, and it carries the guarantee that such a slot is re-executed on recovery. The batch path honours that at :147-151. The serial path can read those same slots and does not.
Before this PR the two never met. PENDING was written only by durableExecuteWithReconcile (RunnerContextImpl.java:612, :618) and the Python bridge's _prepare_reconciler_execution, both gated on a non-null reconciler(), and that method gates its own cache read on !current.isPending() at :622. The completion-only path never needed that gate, because non-reconcilable calls never wrote a PENDING slot. Now they do: ToolCallAction.java:135 never overrides reconciler() and the interface default is null.
CallResult.matches compares only functionId and argsDigest (CallResult.java:160-162), so a reserved slot reads as a cache hit at RunnerContextImpl.java:757, both payloads are null, and tryGetCachedResult throws NPE on return Optional.of(null) at :575. ToolCallAction.java:192 catches it, so the tool is reported failed rather than re-executed.
Two routes onto the serial path after a mid-batch crash, with the reserved slots still checkpointed. Setting tool-call.parallel=false and restarting is one. The other needs no config change: a trailing tool whose resource fails to resolve on the recovery run is dropped at ToolCallAction.java:114, executions.size() falls to 1, and the > 1 guard at :67 routes to serial.
Treating a PENDING slot as a miss in matchNextOrClearSubsequentCallResult / tryGetCachedResult would extend the same re-execute guarantee to that path. Would that be the right place for it, or is there a reason the serial read should keep seeing a hit?
There was a problem hiding this comment.
Good catch! This is indeed a serious issue. I'll fix the problem where non‑reconcilable ones under the serial path contain pending slots. For recovery reruns, I'll use finalizeCallAt to handle the existing pending slots instead of creating a new one, which would otherwise cause another error.
There was a problem hiding this comment.
That plan sounds right. One part I wasn't sure it reaches: the read side is status-blind independently of how the slot got written. CallResult.matches (CallResult.java:160) compares only functionId and argsDigest, so a reserved PENDING slot would still read as a hit at the match site even once finalizeCallAt owns the write side. Does the serial path want a status guard there too, or does your fix remove the route that reaches it with a PENDING slot in the first place?
There was a problem hiding this comment.
Yes, I'll mark it as 'no result read' during the read operation and include this in the next commit. I'm a bit tight on time at the moment, so I plan to push it over the weekend. We can revisit it then~
| for (int i = 0; i < outcomes.size(); i++) { | ||
| recordOutcome(executions.get(i), outcomes.get(i), success, error, responses); | ||
| } | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Any throw out of durableExecuteAllAsync after partial progress marks every tool failed, including slots already durably finalized SUCCEEDED. The action is then marked completed, so those results never surface on a later run. Lost, not delayed.
The serial path just below degrades per tool; this one is all-or-nothing. Routes in besides the Duration conversion on AgentExecutionOptions.java:82: a JsonProcessingException out of finalizeExecutedOutcomes (JavaRunnerContextImpl.java:211), or a deserialization failure in readTerminalOutcomeAt.
Would recording the batch-level exception only against slots with no outcome yet work here, or is failing the whole set the intent?
There was a problem hiding this comment.
I'll handle all exceptions inside durableExecuteAllAsync, so the caller only needs to process the outcome without additional error handling. This keeps the outer logic cleaner.
There was a problem hiding this comment.
Handling it inside durableExecuteAllAsync so the caller only reads outcomes is a cleaner seam, agreed. Where would you draw the line for failures that aren't a tool's fault? A state-store write failure mapped into N per-tool outcomes would let the action complete normally, with the agent carrying on as though "all tools failed" were a real result. Would it be worth letting those keep propagating, and mapping only per-call execution failures into outcomes?
| return partial(call.func, *call.args, **kwargs) | ||
|
|
||
| def _prepare_batch_execution(self, calls: list[DurableCall]) -> _BatchExecutionPlan: | ||
| args_digest = "" |
There was a problem hiding this comment.
The batch keys slots on ("tool-call-<id>", "") (tool_call_action.py:134). The Python single-call path keys on (_compute_function_id(func), args_digest) (:482-483), and since the callable is tool.call, _compute_function_id returns the same string for every tool. Java has no such split: RunnerContextImpl.java:604-605 uses getId() plus "" for the single-call state machine too.
So a job that checkpoints mid-batch and restarts with tool-call.parallel=false replays cleanly on Java, while on Python matchNextOrClearSubsequentCallResult cannot match the persisted key, truncates the journal, and calls every completed tool again. Same flip that reaches the serial-path issue on JavaRunnerContextImpl.java:193.
The per-call functionId rename was headed for its own issue, so not asking for it here. What caught my eye is that the batch path adopted it while the single-call path did not, so the two disagree inside Python today. Is documenting the flip as not recoverable across a restart the right stopgap until that lands?
There was a problem hiding this comment.
Yes, inconsistent IDs on the Python side would cause this issue.
Regarding functionId – I plan to address it in this PR as well. My thought is to make it exactly the same on both the Python and Java sides. What do you think? Alternatively, we can open a separate issue for further discussion if needed.
There was a problem hiding this comment.
Happy either way, but I think two separable things are bundled under functionId, and only one of them is the piece we agreed to defer.
The narrow one is the divergence this PR introduces, which feels in scope here. _build_executions already mints DurableCall(id=f"tool-call-{call_id}") (tool_call_action.py:134), but _execute_sequentially passes only call.func (:174, :180), so the id is dropped and the runtime falls back to _compute_function_id → module.qualname, the same string for every tool. Java has no such split — buildCallables mints one callable and both paths use it. Threading the id that's already built through the serial path looks contained.
The broader one is using a unique per-call id as recovery identity — the "same args, different result" case we split out to its own issue. Worth flagging that this PR already crosses that line on Java: getId() returns the constant "tool-call" on main, and "tool-call-" + id here, for the serial path as well as the batch.
That carries an upgrade cost either way. An action checkpointed mid-tool-loop on the current release and restored on this build recorded functionId="tool-call", so matchNextOrClearSubsequentCallResult (RunnerContextImpl.java:654) won't match, clears from the current index, and the already-completed tool calls run again — surfaced in the log as "Non-deterministic call detected", which would point an operator at the wrong cause.
So my leaning would be to keep the serial-path id threading here as a parity fix and let the separate issue carry the semantics change. If you'd rather land both together, that works too — the piece worth writing down either way is the cross-version recovery behaviour. What's your read?
weiqingy
left a comment
There was a problem hiding this comment.
Thanks for turning these around so fast, and the Long switch makes sense to me. Replies on the individual threads, one new thing inline, and an answer to your functionId question.
| self._ctx.tool_call_executor.submit(supplier) for _, supplier in plan.suppliers | ||
| ] | ||
| timeout_ms = self._ctx.config.get(AgentExecutionOptions.TOOL_CALL_BATCH_TIMEOUT) | ||
| deadline = time.monotonic() + timeout_ms / 1000 if timeout_ms >= 0 else None |
There was a problem hiding this comment.
timeout_ms >= 0 makes a configured 0 mean "deadline is now" rather than "no timeout" — the first check in the loop fires and every batch fails with a TimeoutError before any tool can finish.
Java goes the other way, returning no deadline when timeout.isZero() || timeout.isNegative() (ContinuationActionExecutor.java:255), and the config table added in this PR documents it as "Non-positive disables it" (configuration.md:137). The -1 default keeps this out of the way today, but 0 is the natural thing to write for "no timeout". Should this be > 0?
There was a problem hiding this comment.
Thank you for the careful review~ I'll fix it in the next commit, changing it to > 0
Linked issue: #925
Purpose of change
Add parallel execution capability for tool calls via a new public API (
RunnerContext.durableExecuteAllAsync()), which submits a batch of tool calls concurrently to an async thread pool and returns results in the original order after all complete.The execution is durable: the state of in-flight calls is persisted, so that upon recovery (e.g., from checkpoint) the system correctly restores and completes the pending calls, ensuring no duplicate or lost invocations.
Tests
API
RunnerContext.durableExecuteAllAsync()for batch parallel execution of tool calls.Documentation
doc-neededdoc-not-neededdoc-included