Repository navigation
Conversation
EnvironmentKeyAuthFilter calls ControlPlaneClient.verifyEnvironmentKey from
the reactive security filter chain (reactor-http-epoll thread). The WebClient
chain inside ends with .block(), which Reactor rejects on NonBlocking threads,
so verification always failed and every self-hosted environment worker request
was rejected with 401:
verifyEnvironmentKey failed for env_xxx: block()/blockFirst()/blockLast()
are blocking, which is not supported in thread reactor-http-epoll-4
Run the blocking verification on a small dedicated thread pool instead.
SelfHostedWorkerController and WorkerEnvironmentController wrap blocking work
(session resolve via ControlPlaneClient, work queue ops) in Mono.fromCallable
without subscribeOn, so the callable runs on the subscribing parallel/event-loop
thread and fails with:
resolveSession failed for sess_xxx: block()/blockFirst()/blockLast()
are blocking, which is not supported in thread parallel-1
The pending-tools endpoint returned 502 and work/poll failed intermittently,
which made the self-hosted hands worker unusable. Add
.subscribeOn(Schedulers.boundedElastic()) like the other data-plane
controllers (DataSessionApiController) already do.
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
CLA Not Signed The Contributor License Agreement (CLA) check is currently pending on this PR ( @vipzj please sign the CLA via the CLA assistant badge in the comment above, or visit https://cla-assistant.io/agentscope-ai/agentscope-java. Once signed, the Automated check by github-manager-bot |
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Wraps the blocking bodies of the self-hosted worker endpoints (pending-tools, tool-results, skills, work/poll, work, work/stats, work/{id}) in Mono.fromCallable(...).subscribeOn(Schedulers.boundedElastic()), which is the right mechanism for the "blocking call on a NonBlocking thread" class of failure and matches how the rest of the dataplane module already handles it (DataSessionApiController, SessionTurnRunner, SessionEventLog).
Two gaps before this can be called a fix for the reported symptom, plus one capacity note — details inline.
Findings
- [Critical]
SelfHostedWorkerController.java:82— the 401 root cause described in the issue text sits inEnvironmentKeyAuthFilter→ControlPlaneClient.verifyEnvironmentKey, which still calls.block()on the event loop; that file is not in this diff, so verification keeps failing and the filter still falls through to 401 before any of thesesubscribeOncalls are reached. - [Warning]
WorkerEnvironmentController.java:93— the 25 s long-poll now occupies a sharedboundedElasticworker per waiting client; that pool is also used by the session/turn paths, so a handful of workers can move the failure from "401/502" to app-wide thread starvation. - [Warning]
WorkerEnvironmentController.java:143— the write-side worker endpoints in the same controller (ack,heartbeat,stop,updateWork, allMono.fromRunnableon the same blocking queue) were not offloaded, andackis on the poll → ack → tool-results hot path this PR is trying to make usable.
Suggestions
A single shared scheduler constant (or a tiny blocking(Mono) helper) in the dataplane module would cover both the missing endpoints and the repetition across the seven handlers, e.g. mirroring DataSessionApiController.BLOCKING.
Status
- CLA: not signed yet (
license/cla= pending on head73ce3f9c) — this PR cannot be approved or merged until the Contributor License Agreement is completed, so this review is a comment only. - CI at head:
build (ubuntu-latest)/build (windows-latest)/Check License/Check Module Syncpassing;codecov/patchfailing (coverage, not a test failure). mergeable=true,mergeable_state=blocked(branch protection, not a conflict).
No re-review chore required from you on my side — just ping me once the filter change and the CLA land and I will re-check the whole worker path in one pass.
Automated review by github-manager-bot
| requireEnvironmentWorker(auth, environmentId, sessionId); | ||
| return pendingHandsToolService.listPending(sessionId); | ||
| }) | ||
| .subscribeOn(Schedulers.boundedElastic()); |
There was a problem hiding this comment.
This moves the handler bodies off the event loop, but failure #1 in the description (401 on every worker request) lives upstream of them and is not touched here: EnvironmentKeyAuthFilter.filter calls matchesApiKey -> apiKeyVerifier.matches eagerly, and the bound method is ControlPlaneClient::verifyEnvironmentKey, which still ends in bodyToMono(Map.class).block() (ControlPlaneClient.java:496-511). A WebFlux WebFilter runs on the Netty event loop, so that block() still throws block()/blockFirst()/blockLast() are blocking, which is not supported in thread reactor-http-epoll-N, the catch swallows it as return false, and the filter falls through without setting env:<id> — the controller then 401s before any of these subscribeOn operators are reached. The PR's file list is only these two controllers, so no worker route can actually authenticate yet. Is a second commit (making the filter reactive, e.g. Mono.fromCallable(...).subscribeOn(boundedElastic()) around the verification, or a reactive verifyEnvironmentKey) missing from the push?
| HttpStatus.SERVICE_UNAVAILABLE, "Poll interrupted"); | ||
| } | ||
| }) | ||
| .subscribeOn(Schedulers.boundedElastic()); |
There was a problem hiding this comment.
poll is a 25 s long-poll by default, and Schedulers.boundedElastic() is the shared pool — the same one DataSessionApiController (BLOCKING), SessionTurnRunner, SessionEventLog and InternalToolConfirmationController subscribe on, capped at roughly 10x core-count workers. A handful of self-hosted workers polling concurrently can therefore park every boundedElastic worker and turn the 401 into app-wide latency. Two options worth considering: a dedicated newBoundedElastic("worker-poll", ...) scheduler for this route, or pushing the queue poll down to a non-blocking Mono/Sinks wait so a waiting worker costs no thread at all. With subscribeOn as written the blocking behaviour is fixed but the capacity ceiling is just relocated.
| new ResponseStatusException( | ||
| HttpStatus.NOT_FOUND, | ||
| "Unknown work item: " + workId))) | ||
| .subscribeOn(Schedulers.boundedElastic()); |
There was a problem hiding this comment.
The write-side endpoints in this controller are still on the event loop: ack (~L156), heartbeat (~L182), stop (~L189) and updateWork (~L198) return Mono.fromRunnable without subscribeOn, and they call the same blocking queue (workQueue.get/ack/heartbeat/stop). ack is on the hot path of the exact loop this PR is trying to unblock — poll -> ack -> execute -> tool-results — so the 502 class the description reports should remain reachable there. Same treatment (or the shared helper noted below) would make the fix uniform.
Problem
The self-hosted environment (type
self_hosted) hands execution to an externalHandsWorkerMainworker, but on currentmainthe whole worker path is broken — every worker API call fails before doing any work:EnvironmentKeyAuthFiltercallsControlPlaneClient.verifyEnvironmentKeyfrom the reactive security filter chain (areactor-http-epollthread). The WebClient call inside ends with.block(), which Reactor rejects onNonBlockingthreads:Verification therefore always returns
falseand the filter never sets theenv:<id>authentication, sopending-tools/tool-results/work/pollall answer 401.SelfHostedWorkerControllerandWorkerEnvironmentControllerwrap blocking work (session resolve throughControlPlaneClient, work-queue ops) inMono.fromCallablewithoutsubscribeOn, so the callable runs on the subscribingparallel-*thread:GET /api/environments/{id}/sessions/{sid}/pending-toolsreturns 502 andwork/pollfails intermittently.Fix
ControlPlaneClient: run the blocking key verification on a small dedicated daemon thread pool instead of the event-loop thread..subscribeOn(Schedulers.boundedElastic()), matching what the other data-plane controllers (DataSessionApiController) already do — the class javadoc ofControlPlaneClientexplicitly requires callers to schedule off the event loop.Verification
Deployed the data plane with these patches and verified the full flow end to end with a real
HandsWorkerMainworker:pending-tools/tool-results/work/pollreturn 200 with valid environment keys (X-Builder-Environment-Key);self_hostedenvironment suspends withsession.requires_action (tool_suspended)as designed;execute+write_filelocally, posts results, and the suspended turn resumes and produces the finalagent.message.mvn spotless:checkandmvn -pl agentscope-service/service-dataplane packagepass.