Skip to content

feat: implement AI orchestrator request interceptor API - #9879

Merged
ugur-vaadin merged 6 commits into
mainfrom
feat/prompt-preprocessor-research
Aug 19, 2026
Merged

feat: implement AI orchestrator request interceptor API#9879
ugur-vaadin merged 6 commits into
mainfrom
feat/prompt-preprocessor-research

Conversation

@ugur-vaadin

Copy link
Copy Markdown
Contributor

Description

Description pending

Fixes https://github.com/orgs/vaadin/projects/103/views/1?filterQuery=&pane=issue&itemId=224397417

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/latest/contributing/overview
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.
  • Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

@vaadin-review-bot vaadin-review-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ugur-vaadin

Copy link
Copy Markdown
Contributor Author

⚠️ 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.

Updated the JavaDocs.

@ugur-vaadin
ugur-vaadin marked this pull request as ready for review August 14, 2026 07:57
@ugur-vaadin
ugur-vaadin requested a review from tomivirkki August 14, 2026 07:57

@vaadin-review-bot vaadin-review-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ugur-vaadin
ugur-vaadin requested a review from tomivirkki August 18, 2026 07:37

@vaadin-review-bot vaadin-review-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 @NpmPackage version 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 @NpmPackage bump again (that posts a fresh block).
  • If the PR is merged or closed before the 24 hours elapse, the auto-unblock simply does nothing.

@ugur-vaadin
ugur-vaadin marked this pull request as draft August 18, 2026 15:02
@ugur-vaadin
ugur-vaadin force-pushed the feat/prompt-preprocessor-research branch from 3c5a3f0 to e3e9e0d Compare August 18, 2026 16:56
@ugur-vaadin
ugur-vaadin marked this pull request as ready for review August 18, 2026 16:56
@ugur-vaadin
ugur-vaadin enabled auto-merge August 18, 2026 16:57
@sonarqubecloud

Copy link
Copy Markdown

@vaadin-review-bot vaadin-review-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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()) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 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 doPrompttry { 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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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

@vaadin-review-bot
vaadin-review-bot dismissed their stale review August 19, 2026 15:00

24 hours have passed since the @NpmPackage block. Auto-unblocking and approving.

@vaadin-review-bot vaadin-review-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automatically approved 24 hours after the @NpmPackage version bump block.

@ugur-vaadin
ugur-vaadin added this pull request to the merge queue Aug 19, 2026
Merged via the queue into main with commit 21cfdaa Aug 19, 2026
21 checks passed
@ugur-vaadin
ugur-vaadin deleted the feat/prompt-preprocessor-research branch August 19, 2026 15:05
@vaadin-bot

Copy link
Copy Markdown
Collaborator

This ticket/PR has been released with Vaadin 25.3.0-alpha9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants