fix(middleware): avoid event-loop blocking in WorkspaceContextMiddleware onSystemPrompt - #2632
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR addresses #2578 by ensuring WorkspaceContextMiddleware.onSystemPrompt() does not assemble workspace context on the caller thread (which can be a Netty event-loop thread under WebFlux), moving that potentially blocking work onto Reactor’s bounded-elastic scheduler.
Changes:
- Wrap workspace context assembly in
Mono.fromCallable(...)and schedule it viasubscribeOn(Schedulers.boundedElastic()). - Add a regression test to verify workspace context reads do not execute on the caller thread.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| agentscope-harness/src/main/java/io/agentscope/harness/agent/middleware/WorkspaceContextMiddleware.java | Moves workspace context assembly off the caller thread using bounded-elastic scheduling. |
| agentscope-harness/src/test/java/io/agentscope/harness/agent/middleware/WorkspaceContextMiddlewareMemoryPromptTest.java | Adds a regression test asserting workspace reads occur on a different thread than the caller. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This PR moves the blocking workspace-context assembly (filesystem reads for AGENTS.md, MEMORY.md, knowledge files) off the reactive subscriber thread by wrapping buildWorkspaceSection() in Mono.fromCallable().subscribeOn(Schedulers.boundedElastic()). The intent is sound — preventing event-loop blocking when workspace files are read synchronously. A new test validates that the callable indeed executes on a different thread than the caller. However, the removal of the empty-section early-return guard and the lack of onErrorResume for graceful degradation are worth addressing before merge.
| return base + separator + section; | ||
| }) | ||
| .subscribeOn(Schedulers.boundedElastic()); | ||
| } |
There was a problem hiding this comment.
[major] Missing onErrorResume for graceful degradation. buildWorkspaceSection() performs multiple filesystem reads. If any throws, the Mono.fromCallable will propagate the error through the middleware chain, failing the entire agent call. For a supplementary-context middleware, this is overly aggressive. Consider adding .onErrorResume(e -> { log.warn(...); return Mono.just(currentPrompt); }) after subscribeOn to degrade gracefully.
| String separator = base.isEmpty() || base.endsWith("\n") ? "" : "\n"; | ||
| return Mono.just(base + separator + section); | ||
| return Mono.fromCallable( | ||
| () -> { |
There was a problem hiding this comment.
[minor] Removed empty-section early-return guard. The original code had if (section.isEmpty()) return Mono.just(currentPrompt) which short-circuited when no workspace files existed. The new code always concatenates the section (even if empty), which may append an unnecessary trailing newline. Consider restoring the isEmpty guard inside the callable.
| @TempDir Path workspace; | ||
|
|
||
| @Test | ||
| void onSystemPromptBuildsWorkspaceContextOnBoundedElastic() { |
There was a problem hiding this comment.
[nitpick] The test assertion assertFalse(callerThread[0]) is somewhat fragile — it only checks that the caller thread differs from the current thread at assertion time. If the test happens to run on the bounded-elastic scheduler itself, the assertion would still pass but the intent (offloading to a different scheduler) would be better captured by checking the thread name contains 'boundedElastic'.
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This PR moves the blocking workspace-context assembly (filesystem reads for AGENTS.md, MEMORY.md, knowledge files) off the reactive subscriber thread by wrapping buildWorkspaceSection() in Mono.fromCallable().subscribeOn(Schedulers.boundedElastic()). The intent is sound — preventing event-loop blocking when workspace files are read synchronously. A new test validates that the callable indeed executes on a different thread than the caller. However, the removal of the empty-section early-return guard and the lack of onErrorResume for graceful degradation are worth addressing before merge.
| return base + separator + section; | ||
| }) | ||
| .subscribeOn(Schedulers.boundedElastic()); | ||
| } |
There was a problem hiding this comment.
[major] Missing onErrorResume for graceful degradation. buildWorkspaceSection() performs multiple filesystem reads. If any throws, the Mono.fromCallable will propagate the error through the middleware chain, failing the entire agent call. For a supplementary-context middleware, this is overly aggressive. Consider adding .onErrorResume(e -> { log.warn(...); return Mono.just(currentPrompt); }) after subscribeOn to degrade gracefully.
| String separator = base.isEmpty() || base.endsWith("\n") ? "" : "\n"; | ||
| return Mono.just(base + separator + section); | ||
| return Mono.fromCallable( | ||
| () -> { |
There was a problem hiding this comment.
[minor] Removed empty-section early-return guard. The original code had if (section.isEmpty()) return Mono.just(currentPrompt) which short-circuited when no workspace files existed. The new code always concatenates the section (even if empty), which may append an unnecessary trailing newline. Consider restoring the isEmpty guard inside the callable.
| @TempDir Path workspace; | ||
|
|
||
| @Test | ||
| void onSystemPromptBuildsWorkspaceContextOnBoundedElastic() { |
There was a problem hiding this comment.
[nitpick] The test assertion assertFalse(callerThread[0]) is somewhat fragile — it only checks that the caller thread differs from the current thread at assertion time. If the test happens to run on the bounded-elastic scheduler itself, the assertion would still pass but the intent (offloading to a different scheduler) would be better captured by checking the thread name contains 'boundedElastic'.
|
@zouyx 有空再看下: 删除不可达/无意义的空 section 分支,另一处是补了 currentPrompt 为 null 和不以换行结尾时的测试 |
Background
WorkspaceContextMiddleware.onSystemPrompt()assembled workspace context synchronously on the caller thread #2578. In WebFlux deployments, that caller can be a Netty event-loop thread.This PR fixes the issue by moving that workspace context assembly work off the caller thread.
Other middleware paths are not changed here; they need a separate audit and shared blocking-boundary policy.
What's Changed
Mono.fromCallable(...).Schedulers.boundedElastic().