fix(core): offload agent execution setup from caller thread - #2464
fix(core): offload agent execution setup from caller thread#2464Sparkle6979 wants to merge 3 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
oss-maintainer
left a comment
There was a problem hiding this comment.
Review: Approve ✅
Clean fix for #2462 — correctly offloads potentially blocking beforeAgentExecution setup from the caller/event-loop thread onto Schedulers.boundedElastic().
Analysis
Problem: runLifecycleBody called beforeAgentExecution synchronously, which triggers AgentStateStore.get(). Backed by Redis/MySQL/filesystem, this blocks the caller thread — problematic in event-loop style callers.
Solution: Wrap beforeAgentExecution + bindRequestState in Mono.fromCallable(...).subscribeOn(Schedulers.boundedElastic()). The PreparedExecution record cleanly carries the scope out of the callable.
Observations
- Correctness ✅ — The
scope == nullpath is preserved:PreparedExecutionwraps null, and the flatMap branch skipscontextWritewhen scope is null. Semantically identical to the original. - Scheduler choice ✅ —
boundedElasticis the right pool for blocking I/O (Redis/MySQL/fs). Does not steal from the computation pool. - Per-key serialization gate ✅ — Unchanged; the existing gate still serializes correctly since
subscribeOnonly affects thefromCallablesegment. - Tests ✅ — 105 lines of new tests covering the threading behavior. Good coverage.
- Scope — Minimal, focused change. 2 files, +140/-18.
Note
CLA is not yet signed — author needs to complete the CLA check before merge.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
oss-maintainer
left a comment
There was a problem hiding this comment.
Review: Offload agent execution setup from caller thread
Verdict: Approved ✅ (pending CLA signature)
Good fix for the event-loop blocking issue reported in #2462. The approach is sound and well-targeted.
Analysis
- Root cause correctly identified —
ReActAgent.beforeAgentExecutionmay synchronously loadAgentStatethroughAgentStateStore#get, which can be backed by Redis/MySQL/COS/filesystem. Running this on the caller/event-loop thread is a blocking risk. - Solution — wrapping
beforeAgentExecution+bindRequestStateinMono.fromCallable(...).subscribeOn(Schedulers.boundedElastic())correctly offloads the blocking work. ThePreparedExecutionrecord cleanly bundles the result. - Serialization gate preserved — the per-key lock is maintained outside the deferred block, so concurrent calls for the same session still serialize correctly.
- Null scope case handled — when
beforeAgentExecutionreturns null, thecontextWriteis skipped as before. - Test coverage — 105-line test validating the offload behavior.
Blocker
⚠️ CLA not signed — the contributor license agreement check is pending. This needs to be resolved before merge.
LGTM once CLA is signed.
3d4f097 to
67404d5
Compare
Summary
Move agent execution preparation onto Reactor bounded elastic threads so potentially blocking setup work does not run on the caller/event-loop thread.
Fixes #2462.
Details
ReActAgent.beforeAgentExecution may synchronously load AgentState through AgentStateStore#get. Some AgentStateStore implementations can be backed by Redis, MySQL, COS, or filesystem storage, so doing this work on the caller thread can block an event-loop style caller.
This change wraps the beforeAgentExecution and graceful-shutdown request binding step in Mono.fromCallable(...).subscribeOn(Schedulers.boundedElastic()), while keeping the existing per-key serialization gate and preserving the case where beforeAgentExecution returns null.
Validation