feat: implement AI orchestrator request interceptor API - #9879
Conversation
vaadin-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the changes — left 3 comments and collected 1 finding below that could not be attached to the diff.
| Finding | |
|---|---|
| ⚡ | The postpone timeout timer is never cancelled when the UI detaches, pinning the UI, session, and attachment bytes on a global scheduler until the timeout elapses. |
ResponseListener's Javadoc still says the error is "any throw between onRequest() and the start of the stream," which is now wrong for interceptor failures. |
|
| 🧹 | RequestContinuation hand-rolls the exact semantics of CompletableFuture, which the module already uses for the same job. |
| 🧹 | The throw t in resumePostponedPrompt cannot reach any caller and only re-reports the error a second time to the session error handler. |
ResponseListener's Javadoc still says the error is "any throw between onRequest() and the start of the stream," which is now wrong for interceptor failures.
This PR updated the parallel AIController.onResponse Javadoc to note that a throwing/failing/timed-out RequestInterceptor fires onResponse before onRequest() ever runs (verified by the throwingInterceptor_* and timeout tests). ResponseListener.java was not in the changed files and still states on line 37 that getError() carries "any throw between AIController#onRequest() and the start of the stream."
A developer relying on the documented contract assumes onRequest already ran whenever getError() is present, and may release per-turn state that was never set up. Add the same note here that the AIController.onResponse doc now carries.
ResponseListener.java:37 · correctness · confirmed
Updated the JavaDocs. |
vaadin-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the changes — left 5 comments.
| Finding | |
|---|---|
| 👀 | On an interception timeout the ResponseListener runs on the shared Schedulers.parallel() pool, where the updated javadoc wrongly promises blocking I/O is safe. |
| 👀 | onResponse header still says it fires "when the LLM stream has completed" and "Every turn fires this exactly once", both now false. |
| 🧹 | RequestContinuation hand-rolls CompletableFuture<Void> plus a manual timeout timer. |
| 👀 | The isProcessing busy guard is claimed once but released at ~9 scattered sites with no single owner. |
| 🧹 | Verdict-drop and cleanup-then-rethrow logic is duplicated between the synchronous path and resumePostponedPrompt. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
vaadin-review-bot
left a comment
There was a problem hiding this comment.
This PR updates the @NpmPackage version. Because the default value of vaadin.npm.minimumFrontendPackageAgeDays is 1, I'm blocking it for now to avoid producing a failed snapshot for other projects.
No action needed — this is fully automated:
- I posted this block automatically when CI detected the
@NpmPackageversion bump. - 24 hours after this message, I will automatically dismiss this review, approve the PR, and enable auto-merge (squash) so it merges as soon as all required checks pass.
What affects the 24h timer:
- New commits / pushes do not reset it — the countdown is anchored to this message, so it keeps running no matter how many times you push.
- The timer only restarts if this block review is manually dismissed and a later CI run detects an
@NpmPackagebump again (that posts a fresh block). - If the PR is merged or closed before the 24 hours elapse, the auto-unblock simply does nothing.
3c5a3f0 to
e3e9e0d
Compare
|
vaadin-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the changes — left 4 comments.
| Finding | |
|---|---|
abortPostponedPrompt clears isProcessing before firing the hooks, so a new prompt's onRequest can be ordered before the aborted turn's onResponse. |
|
postpone accepts any positive Duration, but a sub-millisecond timeout truncates to 0 and fires an immediate timeout. |
|
| 👀 | The isProcessing release is scattered across ~8 code paths guarded only by a warning comment, so a future path that forgets to release wedges the orchestrator forever. |
| 🧹 | The if (!applyInterceptVerdict(event)) { isProcessing.set(false); return; } release-and-return glue is duplicated at both call sites. |
| * leave the orchestrator stuck. | ||
| */ | ||
| private void abortPostponedPrompt(UI ui, Throwable failure) { | ||
| isProcessing.set(false); |
There was a problem hiding this comment.
abortPostponedPrompt clears isProcessing before firing the hooks, so a new prompt's onRequest can be ordered before the aborted turn's onResponse.
A postponed prompt times out (or is fail()ed) on a background thread. abortPostponedPrompt runs isProcessing.set(false) at line 660 and only then calls fireResponseListener, which queues controller.onResponse(TimeoutException) via ui.access. In the window between the release and that queue, the user submits a new prompt on the UI thread; the compareAndSet succeeds and the new turn queues its controller.onRequest() first. On the UI thread the hooks then run as onRequest(new) → onResponse(old), so a controller that acquires a resource in onRequest and frees it in onResponse frees the new turn's resource.
The streaming path avoids this: the onError/onComplete handler (which calls fireResponseListener) runs before Reactor's doFinally releases the flag, so the aborted turn's onResponse is always queued ahead of any new onRequest. Make the abort path match — release in a finally after fireResponseListener, keeping the existing catch (UIDetachedException) so a detached UI still can't leave the flag stuck.
AIOrchestrator.java:660 · correctness · plausible
| */ | ||
| public RequestContinuation postpone(Duration timeout) { | ||
| Objects.requireNonNull(timeout, "Timeout must not be null"); | ||
| if (timeout.isZero() || timeout.isNegative()) { |
There was a problem hiding this comment.
postpone accepts any positive Duration, but a sub-millisecond timeout truncates to 0 and fires an immediate timeout.
An interceptor calls event.postpone(Duration.ofNanos(500_000)) (0.5 ms). The check at RequestInterceptor.java:360 only rejects zero/negative, so this passes. handlePostponedPrompt then arms the timer with timeout.toMillis() (AIOrchestrator.java:632), which truncates 0.5 ms to 0, so Schedulers.boundedElastic().schedule(fail(...), 0, MILLISECONDS) fires the TimeoutException before the async work can call proceed(). Any positive sub-millisecond timeout silently makes the postponed prompt fail on arrival.
Either round the delay up to at least 1 ms, or reject sub-millisecond timeouts in postpone so the documented "positive" contract matches what the timer can honor.
RequestInterceptor.java:360 · correctness · confirmed
| * deserialization. When adding a new path out of a prompt, decide which of | ||
| * these owns the release; a missed release blocks all later prompts. | ||
| */ | ||
| private final AtomicBoolean isProcessing = new AtomicBoolean(false); |
There was a problem hiding this comment.
👀 The isProcessing release is scattered across ~8 code paths guarded only by a warning comment, so a future path that forgets to release wedges the orchestrator forever.
doPrompt claims the flag with one CAS at line 480, but releasing it is spread across nine sites: the stream doFinally (438), the doPrompt catch (495), the verdict-drop paths (524, 688), the resume failure/detach handlers (693, 698), abortPostponedPrompt (660), the handlePostponedPrompt catch (647), and Reconnector#apply (1037). The field comment at line 202 tells maintainers to "decide which of these owns the release" — the invariant is enforced by discipline, not structure. A new exit path that forgets its release leaves isProcessing stuck and drops every later prompt.
A lease/token owned by doPrompt — try { handedOff = process(...); } finally { if (!handedOff) isProcessing.set(false); }, with only the stream and the continuation marked as adopters — makes a missed release impossible instead of merely warned against, and collapses the inline releases at 495/524/647/688/693.
AIOrchestrator.java:210 · altitude · plausible
| return; | ||
| } | ||
| if (!applyInterceptVerdict(event)) { | ||
| isProcessing.set(false); |
There was a problem hiding this comment.
🧹 The if (!applyInterceptVerdict(event)) { isProcessing.set(false); return; } release-and-return glue is duplicated at both call sites.
The exact block appears in processUserInput (523-525) and in resumePostponedPrompt (687-690), each relying on the caller to remember to release isProcessing on the drop path — the same footgun the field comment warns about. applyInterceptVerdict already owns the whole drop decision, so having it release isProcessing on its false return collapses both call sites to if (!applyInterceptVerdict(event)) return; and removes one place a future edit can leave the flag stuck.
AIOrchestrator.java:524 · simplification · plausible
24 hours have passed since the @NpmPackage block. Auto-unblocking and approving.
vaadin-review-bot
left a comment
There was a problem hiding this comment.
Automatically approved 24 hours after the @NpmPackage version bump block.
|
This ticket/PR has been released with Vaadin 25.3.0-alpha9. |



Description
Description pending
Fixes https://github.com/orgs/vaadin/projects/103/views/1?filterQuery=&pane=issue&itemId=224397417
Type of change
Checklist